feat: update accounts:set to work with keychain managers#3696
feat: update accounts:set to work with keychain managers#3696k80bowman wants to merge 4 commits into
Conversation
k80bowman
force-pushed
the
k80/accounts-set
branch
from
b23a224 to
9689970
Compare
michaelmalave
left a comment
michaelmalave
left a comment
There was a problem hiding this comment.
Leaving some comments. Nothing blocking & implementation looks good overall!
| } | ||
|
|
||
| AccountsModule.set(name) | ||
| await AccountsModule.set(name, this.config.dataDir) |
There was a problem hiding this comment.
The match checks both name and username, but the raw input is passed to set() where the netrc path does a file lookup by that string. Should we resolve the matched account to ensure the correct identifier reaches each storage path?
| it('returns an error if the account is not in the list', async function () { | ||
| listStub.resolves([{name: 'test-account', username: 'user1'}, {name: 'test-account-2', username: 'user2'}]) | ||
| await runCommand(Cmd, ['test-account-3']) | ||
| .catch((error: Error) => { |
There was a problem hiding this comment.
Not major but the .catch() pattern validates the error message but doesn't fail if no error occurs. Should we adopt the try/catch + expect.fail() pattern used in newer tests like rake and data/pg to ensure the error path is actually exercised?
| const accounts = await AccountsModule.list() | ||
| const accountExists = accounts.some(account => account.name === name || account.username === name) | ||
|
|
||
| if (!(accountExists)) { |
There was a problem hiding this comment.
So small but probably dont need double parens here. Might be simplified for clarity.
| remove(name: string): void | ||
| set(name: string): Promise<void> | ||
| set(name: string, dataDir: string): Promise<void> | ||
| writeLoginState(configDir: string, name: string): Promise<void> |
There was a problem hiding this comment.
This seems like something IAccountsWrapper wouldn't need to implement. Should we keep it off the interface and stub it a different way in tests?
2 participants
Summary
Updates the
accounts:setcommand to work with keychain managers.Type of Change
Breaking Changes (major semver update)
!after your change type to denote a change that breaks current behaviorFeature Additions (minor semver update)
Patch Updates (patch semver update)
Testing
Notes:
npm i && npm run build./bin/run loginto log in to your first account.heroku accounts:add account1to add this account to your accounts cache for .netrc./bin/run loginto log in to your second accountheroku accounts:add account2to add this second account to your accounts cache for .netrcSteps:
./bin/run accounts:set <username for account 1>./bin/run appsand you should see the apps for account 1./bin/run accounts:set <username for account 2>./bin/run appsand you should see the apps for account 2HEROKU_NETRC_WRITE=true ./bin/run accounts:set account1HEROKU_NETRC_WRITE=true ./bin/run appsand you should see the apps for account 1HEROKU_NETRC_WRITE=true ./bin/run accounts:set account2HEROKU_NETRC_WRITE=true ./bin/run appsand you should see the apps for account 2Screenshots (if applicable)
Related Issues
GUS work item: W-20867209